[nexus] Remove duplicated networking types from `omicron-common` / external API by jgallagher · Pull Request #10344 · oxidecomputer/omicron

jgallagher · 2026-04-29T19:24:29Z

This fixes #10293. 2/3 of these were straightforward; removing the duplicate SwitchPortGeometry2 and TxEqConfig2 types required removing those from common and moving any other types still in common that used them out into nexus-types-versions; those are done in the first two commits on this PR.

Removing the duplicate SwitchInterfaceKind2 was more involved because the two types didn't have the same structure despite having the same name. This is split into two commits: 71f7db2 renames one of them to SwitchInterfaceKindNoVlanDetails and moves it to nexus-types-versions (where it lives alongside SwitchInterfaceKind). Then 9db5a0cad8694f40e1c572f172794b2144ce4bdb makes an API change to SwitchInterfaceConfig (changing the kind to the SwitchInterfaceKind that includes the vlan ID) and SwitchPortSettings.

The SwitchPortSettings change needs the most scrutiny. My understanding is that on main, SwitchPortSettings has these two fields:

    /// Layer 3 interface settings.
    pub interfaces: Vec<SwitchInterfaceConfig>,

    /// Vlan interface settings.
    pub vlan_interfaces: Vec<SwitchVlanInterfaceConfig>,

where each item in interfaces has a kind of either Primary, Loopback, or Vlan. If the kind is Vlan, then there will be an item in vlan_interfaces containing just the interface ID and the vlan ID:

pub struct SwitchVlanInterfaceConfig {
    pub interface_config_id: Uuid,
    pub vlan_id: u16,
}

This is based on both the comments and the path that inserts these into the db, where we only add items to vlan_interfaces if it's coming from an interface with the Vlan kind:

omicron/nexus/db-queries/src/db/datastore/switch_port.rs

Lines 1398 to 1412 in 3e4c5bc

    
           for i in &params.interfaces { 
        
               let ifx_config = SwitchInterfaceConfig::new( 
        
                   psid, 
        
                   i.link_name.clone().into(), 
        
                   i.v6_enabled, 
        
                   i.kind.into(), 
        
               ); 
        
               interface_config.push(ifx_config.clone()); 
        
               if let networking::SwitchInterfaceKind::Vlan(vlan_if) = i.kind { 
        
                   vlan_interface_config.push(SwitchVlanInterfaceConfig::new( 
        
                       ifx_config.id, 
        
                       vlan_if.vid, 
        
                   )); 
        
               } 
        
           }

I assume the expectation here is that clients would have to reassemble these two vecs to glue together the vlan ID of any interface with a vlan by taking the interface ID from interfaces and finding it in vlan_interfaces. The change I made on this PR is to drop the vlan_interfaces field entirely, change the kind of SwitchInterfaceConfig to include the vlan ID, then do the reassembly ourselves when we load this object from the db:

omicron/nexus/db-queries/src/db/datastore/switch_port.rs

Lines 198 to 252 in 4f0da85

    
                   // We expect the db to hold exactly one `vlan_interface` row for any 
        
                   // `interface` that has kind `Vlan`. Build up a map of all the vlan 
        
                   // interface rows; we'll then prune through that map as we loop over all 
        
                   // the interfaces and check that it's empty when we're done. 
        
                   // 
        
                   // We expect to find exactly 1 entry for every `Vlan` interface and 
        
                   // nothing extra; anything else is some kind of internal inconsistency. 
        
                   let mut vlan_by_interface = BTreeMap::new(); 
        
                   for vlan_iface in result.vlan_interfaces { 
        
                       let SwitchVlanInterfaceConfig { interface_config_id, vid } = 
        
                           vlan_iface; 
        
                       vlan_by_interface.insert(interface_config_id, vid); 
        
                   } 
        
                   let mut interfaces = Vec::with_capacity(result.interfaces.len()); 
        
                   for iface in result.interfaces { 
        
                       let kind = match iface.kind { 
        
                           DbSwitchInterfaceKind::Primary => { 
        
                               networking::SwitchInterfaceKind::Primary 
        
                           } 
        
                           DbSwitchInterfaceKind::Loopback => { 
        
                               networking::SwitchInterfaceKind::Loopback 
        
                           } 
        
                           DbSwitchInterfaceKind::Vlan => { 
        
                               let Some(vid) = vlan_by_interface.remove(&iface.id) else { 
        
                                   return Err(external::Error::internal_error(format!( 
        
                                       "switch port settings has inconsistent data: \ 
        
                                        interface {iface:?} has type vlan, but \ 
        
                                        vlan_interfaces does not have matching interface: \ 
        
                                        {vlan_by_interface:?}" 
        
                                   ))); 
        
                               }; 
        
                               networking::SwitchInterfaceKind::Vlan( 
        
                                   networking::SwitchVlanInterface { vid: *vid }, 
        
                               ) 
        
                           } 
        
                       }; 
        
                       interfaces.push(networking::SwitchInterfaceConfig { 
        
                           port_settings_id: iface.port_settings_id, 
        
                           id: iface.id, 
        
                           interface_name: iface.interface_name.into(), 
        
                           v6_enabled: iface.v6_enabled, 
        
                           kind, 
        
                       }); 
        
                   } 
        
                   if !vlan_by_interface.is_empty() { 
        
                       return Err(external::Error::internal_error(format!( 
        
                           "switch port settings has inconsistent data: \ 
        
                            `vlan_interfaces` references interface IDs that do not have \ 
        
                            a matching vlan interface: leftover vlan values: \ 
        
                            {vlan_by_interface:?}, assembled interfaces: {interfaces:?}" 
        
                       ))); 
        
                   }

But please confirm that (a) I understood what these fields contained and (b) the reassembly I'm doing there is correct!

…king-types

internet-diglett · 2026-05-04T20:32:40Z

Starting to look at this today, thank you for your patience!

internet-diglett

LGTM!

internet-diglett · 2026-05-06T21:25:15Z

+
+// TODO-cleanup We could push `LinkFec` and `LinkSpeed` down into
+// sled-agent-types-versions and re-export them instead of having these
+// conversions. That requires <https://github.com/oxidecomputer/drift/pull/20>.


Looks like this has been merged?

So it has! I'll file an issue (this can be done separately and shouldn't affect the external API) and replace the PR link with an issue link. Thanks.

Note that drift 0.1.4 finds an API divergence within omicron that we'll need to fix before updating to it. My plan was to fix it yesterday but then other things happened :)

internet-diglett · 2026-05-07T00:32:08Z

But please confirm that (a) I understood what these fields contained and (b) the reassembly I'm doing there is correct!

I believe you understand them correctly... but it's somewhat of an theoretical exercise because I think we don't actually use those tables today.

@rcgoodfellow

Prior to this PR, we had both `PortFec`+`LinkFec` and `PortSpeed`+`LinkSpeed` (both pairs of enums that were identical except for their names and doc comments). Per @rcgoodfellow in chat, `Link*` is a more accurate name, so this PR keeps that one and removes the `Port*` enums. `LinkFec`/`LinkSpeed` were already used in the Nexus external API, so that one doesn't change at all. A bunch of internal APIs were using the `Port*` version, so those had a lot of wire-compatible churn to include the new docstrings. The diff stat here ignoring OpenAPI spec churn is `+142, -219` (net negative from removing the duplicate types and associated `From` impls). This was originally part of #10344, but was deferred to wait for some `drift` fixes, all of which have now landed, courtesy of @ahl and @sunshowers. Closes #10403.

jgallagher added 8 commits April 29, 2026 11:40

remove duplicated SwitchPortGeometry

39936f2

remove duplicated TxEqConfig

1b847bc

rename SwitchInterfaceKind2

71f7db2

remove SwitchInterfaceKind2

9db5a0c

renumber API version before merging main

b1668ab

Merge remote-tracking branch 'origin/main' into john/move-more-networ…

806ce3a

…king-types

add link to drift PR

70cb5f0

minor cleanup

4f0da85

jgallagher requested review from ahl, internet-diglett and sunshowers April 29, 2026 19:24

rcgoodfellow added the networking Related to the networking. label Apr 30, 2026

internet-diglett approved these changes May 7, 2026

View reviewed changes

jgallagher mentioned this pull request May 7, 2026

Squish Port{Fec,Speed} and Link{Fec,Speed} into one type #10403

Closed

jgallagher added 2 commits May 7, 2026 13:34

Merge branch 'main' into john/move-more-networking-types

5b073b1

replace TODO link

48353ff

jgallagher merged commit 9e56717 into main May 7, 2026
16 checks passed

jgallagher deleted the john/move-more-networking-types branch May 7, 2026 21:16

jgallagher mentioned this pull request May 8, 2026

Remove Port{Fec,Speed} in favor of Link{Fec,Speed} #10417

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[nexus] Remove duplicated networking types from `omicron-common` / external API#10344

[nexus] Remove duplicated networking types from `omicron-common` / external API#10344
jgallagher merged 10 commits into
mainfrom
john/move-more-networking-types

jgallagher commented Apr 29, 2026

Uh oh!

internet-diglett commented May 4, 2026

Uh oh!

internet-diglett left a comment

Uh oh!

internet-diglett May 6, 2026

Uh oh!

jgallagher May 7, 2026

Uh oh!

sunshowers May 7, 2026

Uh oh!

internet-diglett commented May 7, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	for i in &params.interfaces {
	let ifx_config = SwitchInterfaceConfig::new(
	psid,
	i.link_name.clone().into(),
	i.v6_enabled,
	i.kind.into(),
	);
	interface_config.push(ifx_config.clone());
	if let networking::SwitchInterfaceKind::Vlan(vlan_if) = i.kind {
	vlan_interface_config.push(SwitchVlanInterfaceConfig::new(
	ifx_config.id,
	vlan_if.vid,
	));
	}
	}

	// We expect the db to hold exactly one `vlan_interface` row for any
	// `interface` that has kind `Vlan`. Build up a map of all the vlan
	// interface rows; we'll then prune through that map as we loop over all
	// the interfaces and check that it's empty when we're done.
	//
	// We expect to find exactly 1 entry for every `Vlan` interface and
	// nothing extra; anything else is some kind of internal inconsistency.
	let mut vlan_by_interface = BTreeMap::new();
	for vlan_iface in result.vlan_interfaces {
	let SwitchVlanInterfaceConfig { interface_config_id, vid } =
	vlan_iface;
	vlan_by_interface.insert(interface_config_id, vid);
	}

	let mut interfaces = Vec::with_capacity(result.interfaces.len());
	for iface in result.interfaces {
	let kind = match iface.kind {
	DbSwitchInterfaceKind::Primary => {
	networking::SwitchInterfaceKind::Primary
	}
	DbSwitchInterfaceKind::Loopback => {
	networking::SwitchInterfaceKind::Loopback
	}
	DbSwitchInterfaceKind::Vlan => {
	let Some(vid) = vlan_by_interface.remove(&iface.id) else {
	return Err(external::Error::internal_error(format!(
	"switch port settings has inconsistent data: \
	interface {iface:?} has type vlan, but \
	vlan_interfaces does not have matching interface: \
	{vlan_by_interface:?}"
	)));
	};
	networking::SwitchInterfaceKind::Vlan(
	networking::SwitchVlanInterface { vid: *vid },
	)
	}
	};

	interfaces.push(networking::SwitchInterfaceConfig {
	port_settings_id: iface.port_settings_id,
	id: iface.id,
	interface_name: iface.interface_name.into(),
	v6_enabled: iface.v6_enabled,
	kind,
	});
	}

	if !vlan_by_interface.is_empty() {
	return Err(external::Error::internal_error(format!(
	"switch port settings has inconsistent data: \
	`vlan_interfaces` references interface IDs that do not have \
	a matching vlan interface: leftover vlan values: \
	{vlan_by_interface:?}, assembled interfaces: {interfaces:?}"
	)));
	}

Conversation

jgallagher commented Apr 29, 2026

Uh oh!

internet-diglett commented May 4, 2026

Uh oh!

internet-diglett left a comment

Choose a reason for hiding this comment

Uh oh!

internet-diglett May 6, 2026

Choose a reason for hiding this comment

Uh oh!

jgallagher May 7, 2026

Choose a reason for hiding this comment

Uh oh!

sunshowers May 7, 2026

Choose a reason for hiding this comment

Uh oh!

internet-diglett commented May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

internet-diglett commented May 7, 2026 •

edited

Loading